crypto: Improve error checking and reporting #3753

stefanmb · 2015-11-10T23:52:30Z

In FIPS mode several test failures occur because of missing checks for OpenSSL API return codes. Where error checking already existed, the error messages often report some static string instead of passing through the actual error string from OpenSSL (which is significantly more helpful in identifying the root cause of the problem). I’ve added checks where necessary to prevent hard crashes and have given precedence to returning the OpenSSL error strings, instead of generic error strings, whenever possible.

mhdawson · 2015-11-11T18:41:02Z

This looks pretty straight forward. Just one question on this part:

bool DiffieHellman::Init(int primeLength, int g) {
   dh = DH_new();
-  DH_generate_parameters_ex(dh, primeLength, g, 0);
+  if (!DH_generate_parameters_ex(dh, primeLength, g, 0))
+    return false;
   bool result = VerifyContext();
   if (!result)
     return false;

Can you explain how it failed without the change versus with. I'm assuming that VerifyContext() would have returned an error but that checking the result from DH_generate_parameters_ex() makes it return a better error

stefanmb · 2015-11-11T19:03:27Z

@mhdawson

Without that change you will get a segfault. Here is the VerifyContext code:

bool DiffieHellman::VerifyContext() {
  int codes;
  if (!DH_check(dh, &codes))
    return false;
  verifyError_ = codes;
  return true;
}

If the previous call to DH_generate_parameters_ex fails, the dh output parameter will not be populated, and DH_check will segfault. For example:

gdb --args out/Debug/node node/test/parallel/test-crypto-dh-odd-key.js

Program received signal SIGSEGV, Segmentation fault.
0x0000000000c85175 in DH_check (dh=0x1fb1870, ret=0x7fffffffcb9c) at ../deps/openssl/openssl/crypto/dh/dh_check.c:115
115     } else if (BN_is_word(dh->g, DH_GENERATOR_2)) {
(gdb) bt 3
#0  0x0000000000c85175 in DH_check (dh=0x1fb1870, ret=0x7fffffffcb9c) at ../deps/openssl/openssl/crypto/dh/dh_check.c:115
#1  0x00000000015e704b in node::crypto::DiffieHellman::VerifyContext (this=0x1fb2df0) at ../src/node_crypto.cc:4578
#2  0x00000000015e5d36 in node::crypto::DiffieHellman::Init (this=0x1fb2df0, primeLength=32, g=2) at ../src/node_crypto.cc:4251

mhdawson · 2015-11-11T19:23:49Z

Thanks for the details.

lgtm

indutny · 2015-11-11T23:06:27Z

LGTM

cc @nodejs/crypto I would like to have one more LGTM on this, just to be sure

shigeki · 2015-11-12T16:23:28Z

LGTM

mhdawson · 2015-11-12T17:10:41Z

CI run https://ci.nodejs.org/job/node-test-pull-request/709/

mhdawson · 2015-11-12T17:53:38Z

lint failures

Done processing src/node_win32_etw_provider.cc
Done processing src/tty_wrap.cc
Done processing src/node_crypto_clienthello.cc
src/node_crypto.cc:3525: Lines should be <= 80 characters long [whitespace/line_length] [2]
src/node_crypto.cc:4542: Lines should be <= 80 characters long [whitespace/line_length] [2]
Done processing src/node_crypto.cc
Done processing src/node_crypto_bio.cc

Stefan please update commit/test and make sure "make lint" passes then I'll respin the CI run

Added checks where necessary to prevent hard crashes and gave precedence to returning the OpenSSL error strings instead of generic error strings.

stefanmb · 2015-11-12T21:29:40Z

@mhdawson

Fixed!

indutny · 2015-11-12T22:18:04Z

LGTM

mhdawson · 2015-11-12T22:28:19Z

lgtm and follow up CI Run https://ci.nodejs.org/job/node-test-pull-request/714/

Will land once this completes

Added checks where necessary to prevent hard crashes and gave precedence to returning the OpenSSL error strings instead of generic error strings. PR-URL: #3753 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

mhdawson · 2015-11-12T23:14:53Z

Nothing in the CI run looked related (failures mostly job not starting to run as opposed to a test failing)

Landed as b7089f6

Added checks where necessary to prevent hard crashes and gave precedence to returning the OpenSSL error strings instead of generic error strings. PR-URL: #3753 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

MylesBorins · 2015-11-17T01:43:26Z

landed in v4.x-staging in c37a560

Added checks where necessary to prevent hard crashes and gave precedence to returning the OpenSSL error strings instead of generic error strings. PR-URL: #3753 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

bnoordhuis · 2015-12-10T13:35:30Z

src/node_crypto.cc

@@ -3530,6 +3535,9 @@ bool Hash::HashInit(const char* hash_type) {
    return false;
  EVP_MD_CTX_init(&mdctx_);
  EVP_DigestInit_ex(&mdctx_, md_, nullptr);
+  if (0 != ERR_peek_error()) {
+    return false;
+  }


@stefanmb Is there a reason you use ERR_peek_error() here instead of looking at the return value of EVP_DigestInit_ex()?

See #4221 for context.

@bnoordhuis Nope, chalk it up to my unfamiliarity with OpenSSL API. Checking EVP_DigestInit_ex's return code should accomplish the same thing, without breaking #4221.

@stefanmb Checking error stack instead of function return status is wrong as it will generate false positive in case there is previously (intentionally) ignored error. It broke node on Alpine Linux.

+1 for fixing this properly.

(yeah, openssl api is funky...)

@ncopa Thank you for bringing this up. Let's continue the discussion under #4221.

Added checks where necessary to prevent hard crashes and gave precedence to returning the OpenSSL error strings instead of generic error strings. PR-URL: #3753 Reviewed-By: Fedor Indutny <fedor@indutny.com> Reviewed-By: Shigeki Ohtsu <ohtsu@iij.ad.jp> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

mscdex added crypto Issues and PRs related to the crypto subsystem. c++ Issues and PRs that require attention from people who are familiar with C++. labels Nov 11, 2015

stefanmb mentioned this pull request Nov 11, 2015

crypto: Resolve FIPS test failures and API issues #3760

Closed

mhdawson self-assigned this Nov 12, 2015

jasnell added the lts-watch-v4.x label Nov 12, 2015

crypto: Improve error checking and reporting

2eec1ce

Added checks where necessary to prevent hard crashes and gave precedence to returning the OpenSSL error strings instead of generic error strings.

stefanmb force-pushed the fips-cs2-error-checking branch from 0c278d4 to 2eec1ce Compare November 12, 2015 21:29

mhdawson closed this Nov 12, 2015

MylesBorins added lts-landed-on-v4.x and removed lts-watch-v4.x labels Nov 17, 2015

rvagg mentioned this pull request Nov 17, 2015

v4.2.2+1 commit candidates nodejs/Release#57

Closed

bnoordhuis reviewed Dec 10, 2015
View reviewed changes

MylesBorins added the land-on-v4.x label Dec 17, 2015

MylesBorins added land-on-v4.x and removed lts-landed-on-v4.x labels Dec 17, 2015

jasnell mentioned this pull request Dec 17, 2015

Release 4.2.4 Planning #4321

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

crypto: Improve error checking and reporting #3753

crypto: Improve error checking and reporting #3753

stefanmb commented Nov 10, 2015

mhdawson commented Nov 11, 2015

stefanmb commented Nov 11, 2015

mhdawson commented Nov 11, 2015

indutny commented Nov 11, 2015

shigeki commented Nov 12, 2015

mhdawson commented Nov 12, 2015

mhdawson commented Nov 12, 2015

stefanmb commented Nov 12, 2015

indutny commented Nov 12, 2015

mhdawson commented Nov 12, 2015

mhdawson commented Nov 12, 2015

MylesBorins commented Nov 17, 2015

bnoordhuis Dec 10, 2015

stefanmb Dec 10, 2015

ncopa Jan 15, 2016

stefanmb Jan 15, 2016

crypto: Improve error checking and reporting #3753

crypto: Improve error checking and reporting #3753

Conversation

stefanmb commented Nov 10, 2015

mhdawson commented Nov 11, 2015

stefanmb commented Nov 11, 2015

mhdawson commented Nov 11, 2015

indutny commented Nov 11, 2015

shigeki commented Nov 12, 2015

mhdawson commented Nov 12, 2015

mhdawson commented Nov 12, 2015

stefanmb commented Nov 12, 2015

indutny commented Nov 12, 2015

mhdawson commented Nov 12, 2015

mhdawson commented Nov 12, 2015

MylesBorins commented Nov 17, 2015

bnoordhuis Dec 10, 2015

Choose a reason for hiding this comment

stefanmb Dec 10, 2015

Choose a reason for hiding this comment

ncopa Jan 15, 2016

Choose a reason for hiding this comment

stefanmb Jan 15, 2016

Choose a reason for hiding this comment